Skip to content

C++: Mark deprecated overrides as deprecated#3520

Merged
geoffw0 merged 3 commits into
github:masterfrom
dbartol:github/codeql-c-analysis-team/79
May 21, 2020
Merged

C++: Mark deprecated overrides as deprecated#3520
geoffw0 merged 3 commits into
github:masterfrom
dbartol:github/codeql-c-analysis-team/79

Conversation

@dbartol

@dbartol dbartol commented May 19, 2020

Copy link
Copy Markdown

Fixes github/codeql-c-analysis-team#79

The QL compiler is about to be changed to emit a warning when overriding a deprecated predicate. This PR marks the existing overrides of deprecated predicates as deprecated themselves, which avoids the warning.

The Print.qll models seem to preserve the isWideCharDefault() predicate for backwards compatibility, so we can't remove them and must continue overriding them.

The XML.qll override is necessary because both superclasses declare the getName() predicate. One is deprecated, and the other is abstract, so we have to have an override.

C++: Mark deprecated overrides as deprecated
3832d4c 
The QL compiler is about to be changed to emit a warning when overriding a deprecated predicate. This PR marks the existing overrides of deprecated predicates as `deprecated` themselves, which avoids the warning.

The `Print.qll` models seem to preserve the `isWideCharDefault()` predicate for backwards compatibility, so we can't remove them and must continue overriding them.

The `XML.qll` override is necessary because both superclasses declare the `getName()` predicate. One is `deprecated`, and the other is `abstract`, so we have to have an override.
@dbartol dbartol added C++ HIGH PRIORITY Don't work on other tasks if you can work on this task labels May 19, 2020
@dbartol dbartol requested a review from a team as a code owner May 19, 2020 20:35
@rdmarsh2

rdmarsh2 commented May 19, 2020

Copy link
Copy Markdown
Contributor

The XML.qll override is necessary because both superclasses declare the getName() predicate. One is deprecated, and the other is abstract, so we have to have an override.

Does this mean that uses of XMLParent::getName() will not trigger deprecation warnings, but uses of XMLFile::getName() will? Also, with XML.qll being synced between languages, will this cause XMLFile::getName() to be deprecated for languages for which File::getName() doesn't exist?

@rdmarsh2

rdmarsh2 commented May 19, 2020

Copy link
Copy Markdown
Contributor

Actually, File::getName() has been deprecated for over two years, along with a lot of other File methods. I think we can just delete them instead.

@dbartol

dbartol commented May 20, 2020

Copy link
Copy Markdown
Author

Ugh. Good points. @geoffw0 @jbj, can I just remove the long-deprecated File::getName() and avoid this whole mess?

@aschackmull

aschackmull commented May 20, 2020

Copy link
Copy Markdown
Contributor

Ugh. Good points. @geoffw0 @jbj, can I just remove the long-deprecated File::getName() and avoid this whole mess?

That's what I did for Java in #3507

@jbj

jbj commented May 20, 2020

Copy link
Copy Markdown
Contributor

👍 This sounds like a great time to remove those long-deprecated member predicates on File.

@jbj jbj mentioned this pull request May 20, 2020
Merged
geoffw0
geoffw0 reviewed May 20, 2020
View reviewed changes

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are simpler than I'd expecteded.

isWideCharDefault changes LGTM.

I'm not so happy with deprecating XMLFile.getName(). If it disambiguates between deprecated and a non-deprecated parent class predicates I would expect it to inherit non-deprecated status. Seems like a bug that this is being flagged?

I'm happy with removing File.getName() as a solution, but this does trigger a chain reaction of removing other predicates from File.qll. Perhaps that's not a bad thing.

@ginsbach

ginsbach commented May 20, 2020

Copy link
Copy Markdown
Contributor

These changes are simpler than I'd expecteded.

isWideCharDefault changes LGTM.

I'm not so happy with deprecating XMLFile.getName(). If it disambiguates between deprecated and a non-deprecated parent class predicates I would expect it to inherit non-deprecated status. Seems like a bug that this is being flagged?

I'm happy with removing File.getName() as a solution, but this does trigger a chain reaction of removing other predicates from File.qll. Perhaps that's not a bad thing.

Thanks for the suggestion. I have discussed this with my team and changed the PR to only report the warning when all overridden members are deprecated.

@dbartol

dbartol commented May 20, 2020

Copy link
Copy Markdown
Author

I've undone the change to XML.qll.
I removed all of the deprecated predicates from File that were not themselves overrides of other deprecated predicates. That was enough to fix the new warnings and remove some cruft, without turning into a multi-day unravelling of the C++ library.

@dbartol

dbartol commented May 21, 2020

Copy link
Copy Markdown
Author

@geoffw0 Is it OK to merge, then?

geoffw0
geoffw0 approved these changes May 21, 2020
View reviewed changes

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@geoffw0 geoffw0 merged commit 0f4723a into github:master May 21, 2020
RasmusWL added a commit to RasmusWL/codeql that referenced this pull request May 25, 2020
@RasmusWL
Python: Fix (upcoming) deprecation compiler-warnings
32c8dd0 
In a near-future release overriding a deprecated predicate without making as
deprecated would give a compiler warning.

Not fixing the XML one. [I can see that this shouldn't be reported
anymore](github#3520 (comment)), and
it's not safe to remove since it was only marked as deprecated in
e6425bb.
@RasmusWL RasmusWL mentioned this pull request May 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@geoffw0 geoffw0 geoffw0 approved these changes

Assignees

@geoffw0 geoffw0

Labels

C++ HIGH PRIORITY Don't work on other tasks if you can work on this task

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dbartol @rdmarsh2 @aschackmull @jbj @ginsbach @geoffw0